Update objects of agent flow#91
Conversation
Signed-off-by: Quanyi Ma <eli@patch.sh>
|
I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a12075acb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ObjectType::ContextSnapshot => 8, | ||
| ObjectType::Decision => 9, | ||
| ObjectType::Evidence => 10, | ||
| ObjectType::PatchSet => 11, | ||
| ObjectType::Plan => 12, |
There was a problem hiding this comment.
Keep pack type codes for new objects within 3-bit limit
Assigning ObjectType values 8..16 here breaks pack round-tripping for AI objects: pack headers only carry 3 type bits (see read_type_and_size masking with 0x70), so these IDs cannot be represented and will decode as an invalid type. Any attempt to pack ContextSnapshot/Task/Run/etc. objects will produce pack data that cannot be decoded correctly, causing fetch/transfer failures or dropped objects.
Useful? React with 👍 / 👎.
| pub fn header(&self) -> &Header { | ||
| &self.header | ||
| } | ||
| } |
There was a problem hiding this comment.
Expose context snapshot state beyond header metadata
This impl now ends after header(), so external callers can no longer read base_commit_sha/selection_strategy/items/summary or populate items and summary after new(). Because those fields are private, snapshots created through the public API are effectively fixed to an empty payload, which is a functional regression from the previous API surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the internal “agent flow” object model by folding AI process object metadata into the shared ObjectType module, wiring those types into object implementations, and bumping several Rust third-party dependencies (bytes/libc/uuid/flate2/rand*).
Changes:
- Bump vendored Rust crates and workspace deps (e.g.,
uuid1.20.0,bytes1.11.1,libc0.2.181,flate21.1.9,rand_chacha0.10.0 /rand_core0.10.0). - Extend
ObjectTypewith AI object variants and move AI header/actor/artifact definitions intosrc/internal/object/types.rs; removesrc/internal/object/header.rs. - Update pack decoding and AI object modules to use the new
ObjectType/Header types and implementObjectTraitfor agent objects.
Reviewed changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/crates/uuid/1.20.0/BUCK | Vendor update to uuid 1.20.0 artifact + hash. |
| third-party/rust/crates/tokio/1.49.0/BUCK | Point tokio deps to updated bytes/libc versions. |
| third-party/rust/crates/tokio-util/0.7.18/BUCK | Point tokio-util deps to updated bytes version. |
| third-party/rust/crates/sqlx-sqlite/0.8.6/BUCK | Update uuid dep to 1.20.0. |
| third-party/rust/crates/sqlx-postgres/0.8.6/BUCK | Update uuid dep to 1.20.0. |
| third-party/rust/crates/sqlx-mysql/0.8.6/BUCK | Update bytes/uuid deps to new versions. |
| third-party/rust/crates/sqlx-core/0.8.6/BUCK | Update bytes/uuid deps to new versions. |
| third-party/rust/crates/socket2/0.6.1/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/signal-hook-registry/1.4.7/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/sea-query/0.32.7/BUCK | Update uuid dep to 1.20.0. |
| third-party/rust/crates/sea-query-binder/0.7.0/BUCK | Update uuid dep to 1.20.0. |
| third-party/rust/crates/sea-orm/1.1.19/BUCK | Update uuid dep to 1.20.0. |
| third-party/rust/crates/rustix/1.1.3/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/rust_decimal/1.39.0/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/rkyv/0.7.45/BUCK | Update bytes/uuid deps to new versions. |
| third-party/rust/crates/ring/0.17.14/BUCK | Update libc dep to 0.2.181 (macOS). |
| third-party/rust/crates/rand_core/0.10.0/BUCK | Add vendored rand_core 0.10.0 target. |
| third-party/rust/crates/rand_chacha/0.9.0/BUCK | Adjust enabled features for rand_chacha 0.9.0. |
| third-party/rust/crates/rand_chacha/0.10.0/BUCK | Add vendored rand_chacha 0.10.0 target. |
| third-party/rust/crates/rand/0.8.5/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/parking_lot_core/0.9.12/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/num_cpus/1.17.0/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/mio/1.1.1/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/libredox/0.1.10/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/libc/0.2.181/BUCK | Vendor update to libc 0.2.181 artifact + version. |
| third-party/rust/crates/jobserver/0.1.34/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/hyper/1.8.1/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/hyper-util/0.1.19/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/http/1.4.0/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/http-body/1.0.1/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/http-body-util/0.1.3/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/getrandom/0.3.4/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/getrandom/0.2.16/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/flate2/1.1.9/BUCK | Vendor update to flate2 1.1.9 artifact + hash. |
| third-party/rust/crates/errno/0.3.14/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/cpufeatures/0.2.17/BUCK | Update libc dep to 0.2.181 (macOS). |
| third-party/rust/crates/clang-sys/1.8.1/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/cc/1.2.49/BUCK | Update libc dep to 0.2.181. |
| third-party/rust/crates/bytes/1.11.1/BUCK | Vendor update to bytes 1.11.1 artifact + hash. |
| third-party/rust/crates/axum/0.8.8/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/axum-core/0.5.5/BUCK | Update bytes dep to 1.11.1. |
| third-party/rust/crates/android_system_properties/0.1.5/BUCK | Update libc dep to 0.2.181. |
| src/internal/pack/decode.rs | Change decode path to return Option<CacheObject> and attempt to skip AI objects. |
| src/internal/object/types.rs | Add AI object types + AI header/actor/artifact metadata into shared types module. |
| src/internal/object/tree.rs | Implement Tree::get_size (previously TODO). |
| src/internal/object/tool.rs | Switch to new Header/ObjectType types and implement ObjectTrait for ToolInvocation. |
| src/internal/object/task.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Task. |
| src/internal/object/run.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Run. |
| src/internal/object/provenance.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Provenance. |
| src/internal/object/plan.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Plan. |
| src/internal/object/patchset.rs | Switch to new Header/ObjectType types and implement ObjectTrait for PatchSet. |
| src/internal/object/mod.rs | Remove header module export. |
| src/internal/object/header.rs | Remove standalone header module (moved into types.rs). |
| src/internal/object/evidence.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Evidence. |
| src/internal/object/decision.rs | Switch to new Header/ObjectType types and implement ObjectTrait for Decision. |
| src/internal/object/context.rs | Switch to new Header/ObjectType types and implement ObjectTrait for ContextSnapshot; reduce exposed API. |
| src/internal/object/commit.rs | Implement Commit::get_size (previously returned 0). |
| src/errors.rs | Add GitError variants for malformed agent objects. |
| buckal.snap | Update third-party fingerprint lock state for bumped crates. |
| Cargo.toml | Bump dependency versions and rand_chacha version. |
| Cargo.lock | Reflect dependency graph updates (bytes/uuid/libc/flate2/rand*). |
| BUCK | Update root BUCK deps to point to bumped third-party crate targets. |
| where | ||
| Self: Sized, | ||
| { | ||
| serde_json::from_slice(data).map_err(|e| GitError::InvalidObjectInfo(e.to_string())) |
There was a problem hiding this comment.
ContextSnapshot::from_bytes maps JSON decode errors to the generic InvalidObjectInfo, but GitError now has a dedicated InvalidContextSnapshotObject variant. Using the specific variant here (and similarly for the other agent object types) would preserve better error classification and make the new variants actionable.
| serde_json::from_slice(data).map_err(|e| GitError::InvalidObjectInfo(e.to_string())) | |
| serde_json::from_slice(data) | |
| .map_err(|e| GitError::InvalidContextSnapshotObject(e.to_string())) |
| | ObjectType::Run | ||
| | ObjectType::Task | ||
| | ObjectType::ToolInvocation => { | ||
| // Wait for encode to implement corresponding compression |
There was a problem hiding this comment.
decode_pack_object returns Ok(None) for the AI object types without consuming the object's compressed payload or advancing offset. This will desynchronize the reader for the next iteration (it will try to parse zlib-compressed bytes as a new object header). Even if you want to skip caching these objects, you still need to read/skip their (compressed) data and update offset accordingly (ideally via a discard/decompress-to-sink helper to avoid allocating the full Vec).
| // Wait for encode to implement corresponding compression | |
| // We don't yet cache these AI object types, but we must still | |
| // consume their compressed payload to keep the pack stream aligned. | |
| let (_data, raw_size) = Pack::decompress_data(&mut reader, size)?; | |
| *offset += raw_size; | |
| // Finalize CRC32 to consume the hasher state, even though the value | |
| // is currently unused for these types. | |
| let _ = hasher.finalize(); |
| /// Convert an object type to a number. | ||
| pub fn to_u8(&self) -> u8 { | ||
| match self { | ||
| ObjectType::Commit => 1, | ||
| ObjectType::Tree => 2, | ||
| ObjectType::Blob => 3, | ||
| ObjectType::Tag => 4, | ||
| ObjectType::OffsetZstdelta => 5, // Type 5 is reserved in standard Git packs; we use it for Zstd delta objects. | ||
| ObjectType::OffsetDelta => 6, | ||
| ObjectType::HashDelta => 7, | ||
| ObjectType::ContextSnapshot => 8, | ||
| ObjectType::Decision => 9, | ||
| ObjectType::Evidence => 10, | ||
| ObjectType::PatchSet => 11, | ||
| ObjectType::Plan => 12, | ||
| ObjectType::Provenance => 13, | ||
| ObjectType::Run => 14, | ||
| ObjectType::Task => 15, | ||
| ObjectType::ToolInvocation => 16, | ||
| } |
There was a problem hiding this comment.
ObjectType::to_u8 assigns values 8..16 for the new AI object types, but the pack object header encoding only has 3 bits for the type (0..7). Because pack/encode.rs shifts this value into the header’s type bits, values >7 will be truncated and encoded as the wrong type (or even 0), and pack/decode.rs can never read back 8..16 from the header. Consider keeping to_u8 limited to valid pack type ids (1..7) and introducing a separate mapping/extension mechanism for AI objects (e.g., store them as Blob in packs and carry the AI subtype inside the object payload/header).
| /// Context snapshot describing selected inputs. | ||
| /// Captures the selection strategy and content identifiers used by a run. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct ContextSnapshot { | ||
| #[serde(flatten)] | ||
| header: Header, | ||
| base_commit_sha: IntegrityHash, | ||
| selection_strategy: SelectionStrategy, | ||
| #[serde(default)] | ||
| items: Vec<ContextItem>, | ||
| summary: Option<String>, | ||
| } | ||
|
|
||
| impl ContextSnapshot { | ||
| pub fn new( | ||
| repo_id: Uuid, | ||
| created_by: ActorRef, | ||
| base_commit_sha: impl AsRef<str>, | ||
| selection_strategy: SelectionStrategy, | ||
| ) -> Result<Self, String> { | ||
| let base_commit_sha = base_commit_sha.as_ref().parse()?; | ||
| Ok(Self { | ||
| header: Header::new(AiObjectType::ContextSnapshot, repo_id, created_by)?, | ||
| header: Header::new(ObjectType::ContextSnapshot, repo_id, created_by)?, | ||
| base_commit_sha, | ||
| selection_strategy, | ||
| items: Vec::new(), | ||
| summary: None, | ||
| }) | ||
| } | ||
|
|
||
| pub fn header(&self) -> &Header { | ||
| &self.header | ||
| } | ||
| } |
There was a problem hiding this comment.
ContextSnapshot keeps base_commit_sha, selection_strategy, items, and summary as private fields but no longer exposes accessors/mutators for them. As a public type this makes it impossible for callers to inspect or build snapshots beyond the header. Consider restoring read accessors (and any needed builders like add_item / set_summary) or making the fields public if external access is intended.
Signed-off-by: Eli Ma <eli@patch.sh>
|
I'll analyze this and get back to you. |
Signed-off-by: Eli Ma <eli@patch.sh>
|
I'll analyze this and get back to you. |
|
@codex review |
Signed-off-by: Eli Ma <eli@patch.sh>
|
I'll analyze this and get back to you. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: Eli Ma <eli@patch.sh>
|
I'll analyze this and get back to you. |
| | ObjectType::ToolInvocation => { | ||
| // Wait for encode to implement corresponding compression | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
decode_pack_object now uses ObjectType::from_pack_type_u8(type_bits) (pack header 3-bit type field). That decoder can only yield the 1..=7 pack types, so the ObjectType::{ContextSnapshot,..,ToolInvocation} match arm is unreachable. Returning Ok(None) here also wouldn’t advance the reader/offset, which would desync decoding if it ever happened. Suggest removing this arm and instead returning an error for any unsupported pack type bits, or implement a real on-disk encoding for AI objects before introducing a skip path.
| | ObjectType::ToolInvocation => { | |
| // Wait for encode to implement corresponding compression | |
| Ok(None) | |
| } | |
| | ObjectType::ToolInvocation => Err(GitError::InvalidObjectInfo( | |
| format!("Unsupported object type in packfile: {:?}", t), | |
| )), |
| // Because we may skip some objects (e.g. AI objects), we use >= instead of == | ||
| assert!(self.number >= caches.total_inserted()); |
There was a problem hiding this comment.
This assertion was relaxed to >=, which can mask real decode/caching bugs (e.g. dropped objects due to logic errors) because pack object count should match the number of cached base objects/deltas once processing completes. If AI objects are truly being skipped, it would be better to explicitly track skipped and assert self.number == caches.total_inserted() + skipped (and ensure skipped objects are fully consumed from the stream).
| // Because we may skip some objects (e.g. AI objects), we use >= instead of == | |
| assert!(self.number >= caches.total_inserted()); | |
| // At this point all pack objects must be fully accounted for: either inserted into | |
| // caches or explicitly treated as skipped/consumed earlier in the pipeline. Any | |
| // legitimately skipped objects must be reflected in `self.number` (or in a separate | |
| // `skipped` counter incorporated here), rather than weakening this assertion. | |
| assert_eq!(self.number, caches.total_inserted()); |
| with: | ||
| # Use PR head SHA for pull_request_target, fallback to current SHA otherwise | ||
| # Use PR head SHA for pull_request_target to review the actual PR code | ||
| # For comment events, this will default to the base branch (PR context is inferred by Claude action) | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
For issue_comment / pull_request_review_comment / pull_request_review events, github.event.pull_request is not present, so this checkout will fall back to github.sha (default branch commit) and the review will run against the wrong code. The previous workflow explicitly resolved and checked out the PR head ref for comment-driven triggers; that behavior likely needs to be preserved (e.g. resolve PR number via gh pr view and fetch/checkout the head SHA/ref).
| ObjectType::Run => RUN_OBJECT_TYPE, | ||
| ObjectType::Task => TASK_OBJECT_TYPE, | ||
| ObjectType::ToolInvocation => TOOL_INVOCATION_OBJECT_TYPE, | ||
| _ => panic!("can put compute the delta hash value"), |
There was a problem hiding this comment.
ObjectType::to_bytes() panics for the delta variants via the catch-all arm (and the message has a typo: "can put"). Since to_bytes() is used in hashing paths (pack/utils.rs, zlib/stream/inflate.rs), a panic here would be a hard crash if a delta type ever leaks into those callers. Consider making the delta cases explicit (and unreachable with a clear message/invariant), or refactor callers so they can’t request a loose-object header for delta-only types.
| _ => panic!("can put compute the delta hash value"), | |
| ObjectType::OffsetZstdelta | |
| | ObjectType::OffsetDelta | |
| | ObjectType::HashDelta => unreachable!( | |
| "delta object types must not be converted to loose-object type bytes; \ | |
| resolve the base (non-delta) object type before calling ObjectType::to_bytes" | |
| ), |
| /// Malformed context snapshot object. | ||
| #[error("Not a valid agent context snapshot object: {0}")] | ||
| InvalidContextSnapshotObject(String), | ||
|
|
||
| /// Malformed decision object. | ||
| #[error("Not a valid agent decision object: {0}")] | ||
| InvalidDecisionObject(String), | ||
|
|
||
| /// Malformed evidence object. | ||
| #[error("Not a valid agent evidence object: {0}")] | ||
| InvalidEvidenceObject(String), | ||
|
|
||
| /// Malformed patch set object. | ||
| #[error("Not a valid agent patch set object: {0}")] | ||
| InvalidPatchSetObject(String), | ||
|
|
||
| /// Malformed plan object. | ||
| #[error("Not a valid agent plan object: {0}")] | ||
| InvalidPlanObject(String), | ||
|
|
||
| /// Malformed provenance object. | ||
| #[error("Not a valid agent provenance object: {0}")] | ||
| InvalidProvenanceObject(String), | ||
|
|
||
| /// Malformed run object. | ||
| #[error("Not a valid agent run object: {0}")] | ||
| InvalidRunObject(String), | ||
|
|
||
| /// Malformed task object. | ||
| #[error("Not a valid agent task object: {0}")] | ||
| InvalidTaskObject(String), | ||
|
|
||
| /// Malformed tool invocation object. | ||
| #[error("Not a valid agent tool invocation object: {0}")] | ||
| InvalidToolInvocationObject(String), | ||
|
|
There was a problem hiding this comment.
These new Invalid*Object error variants are currently unused: the AI object from_bytes implementations still map JSON decode failures into the generic InvalidObjectInfo. Either wire the new variants into each object’s parsing (for more actionable errors) or remove them to avoid API/enum bloat.
| /// Malformed context snapshot object. | |
| #[error("Not a valid agent context snapshot object: {0}")] | |
| InvalidContextSnapshotObject(String), | |
| /// Malformed decision object. | |
| #[error("Not a valid agent decision object: {0}")] | |
| InvalidDecisionObject(String), | |
| /// Malformed evidence object. | |
| #[error("Not a valid agent evidence object: {0}")] | |
| InvalidEvidenceObject(String), | |
| /// Malformed patch set object. | |
| #[error("Not a valid agent patch set object: {0}")] | |
| InvalidPatchSetObject(String), | |
| /// Malformed plan object. | |
| #[error("Not a valid agent plan object: {0}")] | |
| InvalidPlanObject(String), | |
| /// Malformed provenance object. | |
| #[error("Not a valid agent provenance object: {0}")] | |
| InvalidProvenanceObject(String), | |
| /// Malformed run object. | |
| #[error("Not a valid agent run object: {0}")] | |
| InvalidRunObject(String), | |
| /// Malformed task object. | |
| #[error("Not a valid agent task object: {0}")] | |
| InvalidTaskObject(String), | |
| /// Malformed tool invocation object. | |
| #[error("Not a valid agent tool invocation object: {0}")] | |
| InvalidToolInvocationObject(String), |
| } | ||
|
|
||
| impl Display for ContextSnapshot { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { |
There was a problem hiding this comment.
The Display impl signature uses &mut std::fmt::Formatter without the required lifetime parameter. Formatter is Formatter<'a>, so this should be &mut std::fmt::Formatter<'_> (or import fmt and use fmt::Formatter<'_>), otherwise this won’t compile.
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { |
* Update objects of agent flow Signed-off-by: Quanyi Ma <eli@patch.sh> * Fix zstdelta test Signed-off-by: Eli Ma <eli@patch.sh> * Fix copilot warnings Signed-off-by: Eli Ma <eli@patch.sh> * Update claude review action Signed-off-by: Eli Ma <eli@patch.sh> * Update claude review action Signed-off-by: Eli Ma <eli@patch.sh> --------- Signed-off-by: Quanyi Ma <eli@patch.sh> Signed-off-by: Eli Ma <eli@patch.sh>

No description provided.